Various memory optimizations #94
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Oops, this failed to build on Rust <1.79 because the compiler cannot decide if If bumping MSRV to version this recent is not acceptable, keeping |
A lot of strings and arrays used in nftables structures are static, but currently we need to do allocations everywhere using owned String and Vec. Replace them with Cow so we can pass &'static str or &'static [_] whenever possible, while returning owned value when deserializing.
This evens out some of the large enums' sizes (e.g. Expression and Statement's) from over 200 bytes to <=144. Further reduction could be done, as this commit only changes the obvious ones for now. Also moves BinaryOperation's Box up one level so it only uses one Box.
Since we only expect two elements from Range, we can just use array instead.
This fails to build on 1.65:
```
error[E0277]: the trait bound `[SetItem]: ToOwned` is not satisfied in `Expression`
--> src/stmt.rs:245:14
|
245 | pub dev: Option<Expression>,
| ^^^^^^^^^^^^^^^^^^ within `Expression`, the trait `ToOwned` is not implemented for `[SetItem]`, which is required by `Expression: Sized`
|
= help: the trait `ToOwned` is implemented for `[T]`
note: required because it appears within the type `Expression`
--> src/expr.rs:10:10
|
10 | pub enum Expression {
| ^^^^^^^^^^
note: required by an implicit `Sized` bound in `std::option::Option`
--> /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/option.rs:572:1
...and many, many similar errors
```
It compiles successfully on Rust >=1.79, so this commit can be reverted once our MSRV reaches it.
|
Fixed MSRV build and clippy. Please have another look :) |
|
Amazing @hack3ric! Please allow us some more days to review it. |
JKRhb
reviewed
Nov 24, 2024
Comment on lines
+86
to
+87
| table: Cow::Borrowed("some_inet_table"), | ||
| chain: Cow::Borrowed("some_inet_chain"), |
There was a problem hiding this comment.
Would it also be possible to use .into() in places like these?
There was a problem hiding this comment.
impl<'a> From<&'a str> for Cow<'a, str> (and its fellows) is not const for now so .into() will result in a temporary value (as seen by rustc, even if it is 'static), therefore cannot be borrowed 'static. In this example both the outer slice and the inner str needs to be borrowed 'static, leading to the following error:
error[E0716]: temporary value dropped while borrowed
--> tests/json_tests.rs:57:33
|
57 | objects: Cow::Borrowed(&[
| ________________________________-^
| |_________________________________|
58 | || NfObject::ListObject(NfListObject::Table(Table {
59 | || family: NfFamily::INet,
60 | || name: "some_inet_table".into(),
... ||
106 | || })),
107 | || ]),
| || ^
| ||_________|
| |_________creates a temporary value which is freed while still in use
| cast requires that borrow lasts for `'static`
108 | };
| - temporary value is freed at the end of this statement
If all of the outer containers are Cow::Owned, we can use .into() inside. Otherwise we probably need to stick to manually use Cow::Borrowed. I've also pushed a commit that allows temporary arrays to be used by replacing 'static with 'a. It will then just need to move the array in Cow::Borrowed(&[..]) to another local variable to allow the usage of .into():
let objects = [
NfObject::ListObject(NfListObject::Table(Table {
family: NfFamily::INet,
name: "some_inet_table".into(), // here
handle: None,
})),
// ...
];
let expected = Nftables {
objects: objects.into(); // and here
};
|
Added one commit that lifts |
This adds a bunch of lifetimes, but it works with `.into()` when a borrowed value's array container is stored in another place, without the need of `Cow::Borrowed`:
```
let objects = [
NfObject::ListObject(NfListObject::Table(Table {
family: NfFamily::INet,
name: "some_inet_table".into(), // here
handle: None,
})),
// ...
];
let expected = Nftables {
objects: objects.into(); // and here
};
```
However, `Cow::{Borrowed, Owned}` still needs to be used when you want to write the above in one statement.
|
Clippy issue should be fixed now. |
|
FYI I have another patch ready for upstream, but it depends on this PR: 52902cd. I will submit another PR after this and I hope this too can get into v0.6.0 too :) |
|
I performed a trivial memory benchmark of current main branchcow branch |
jwhb
approved these changes
Dec 10, 2024
| @@ -560,17 +560,17 @@ pub struct FlowTable { | |||
| /// The *devices* are specified as iifname(s) of the input interface(s) of the traffic that should be offloaded. | |||
| /// | |||
| /// Devices are required for both traffic directions. | |||
| /// Vec of device names, e.g. `vec!["wg0".to_string(), "wg0".to_string()]`. | |||
| pub dev: Option<Vec<String>>, | |||
| /// Cow slice of device names, e.g. `vec!["wg0".into(), "wg1".into()].into()`. | |||
|
LGTM! I am afraid of the impact of the breaking changes for users of this crate, but this is obviously the way forward. |
JKRhb
approved these changes
Dec 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See each commit message for details.
These are a lot of breaking changes, so please take your time to check if they are properly done :)